src: use cgroups to get memory limits#27508
Conversation
nodejs-github-bot
added
c++
|
Shouldn't this be done at the libuv layer in its |
|
IMO, heuristics about resource management of the runtime belong in Node, specially once we expand this to tune the background thread-pool based on the number of available CPU cores. The act of reading/parsing the cgroups file could be in libuv, but it wouldn't fit into the |
I recently rewrote the linux implementation of |
|
I would expect |
|
I agree that it would also make sense to have this in the libuv layer; let me explore that some more. And yes, I also agree it would be a different API. |
|
#27805 seems to use |
|
Yes, I was planning to update this PR once libuv 1.29 landed on master (which I now realize it has). |
|
@kjin Yes, I see that you are the author of |
|
Under which exact circumstances does this increase the memory ceiling? libuv doc states:
Also,
So this doesn't seem to be a throw-in replacement to |
|
@ChALkeR Ah, I just updated it to take the min of Edit: Ack, I should also make sure to check that it's non-zero, doing that now. |
|
@kjin And what about it possibly being zero? (Sorry, I updated the comment in #27508 (comment) shortly after I posted it). |
|
@ChALkeR Ok, I've updated that, thanks. |
ofrobots
added
the
notable-change
There was a problem hiding this comment.
Ok, given the current state of libuv/libuv#2323, I expect that cgroup2 support will land in libuv in the foreseeable future.
I am fine with merging this to master at this point.
That said, if we want tests for this, we probably need libuv/libuv#2323 to land first. But even then, the tests would be effective only on cgroup2 systems, and I think we don't have those on CI, so I wouldn't block on that.
@kjin Thanks!
ChALkeR
removed
the
tsc-agenda
|
@ChALkeR Thank you! Yes, I'm working on addressing your comments on libuv/libuv#2323. |
|
Given that I'm still actively working on libuv/libuv#2323, is it possible to land this, or are there any other action items I need to take before this can land? |
|
I think this is good to land, given all the approvals and no standing concerns. |
This change adds the ability to set the memory ceiling for a Node.js process according to a memory limit set by cgroups (via uv_get_constrained_memory), which is used by docker containers to set resource constraints. Previously we would use the physical memory size to estimate the necessary V8 heap sizes, but the physical memory size is not necessarily the correct limit, e.g. if the process is running inside a docker container or is otherwise constrained. Non-Linux systems shouldn't be affected.
|
Landed in 285e036. |
|
Question, how does this work with forks/threads? |
|
@ronag forks share the same cgroup and the same limit here, afaik. It would be hard to introduce some more complex for forks, and it wouldn't be an uncontroversial improvement like this change is. |
12 participants
This PR improves the way we set the memory ceiling for a Node.js process. Previously we would use the physical memory size (from libuv) to estimate the necessary V8 heap sizes. The physical memory size is not necessarily the correct limit, e.g. if the process is running inside a docker container or is otherwise constrained.
This change adds the ability to get a memory limit set by linux cgroups, which is used by docker containers to set resource constraints.
I've created a "resource limits" namespace/class that maybe should be the interface through which all resource constraint parameters are retrieved. So far that is just the memory limit but this could be expanded to other cgroups controller params. Also, this functionality is not necessarily specific to linux, so it's possible to have other implementations for other platforms (though this change is primarily motivated by the docker use case). Currently, behavior for other platforms besides linux is unchanged.
See also this article about support for cgroups in Java.
cc/ @ofrobots
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes